Fix opcode parsing, interface instanceof, and runtime bugs#159
Merged
Conversation
- invokeinterface/invokedynamic now consume all four operand bytes - ClassDefinition exposes interface names; is_inherited_from checks interfaces - putstatic narrows int stack values to the field type like putfield - JavaLangString::to_rust_string no longer panics on unpaired surrogates - ByteArrayInputStream.mark saves position instead of readlimit - InputStream.skip returns actual skipped bytes with bounded buffer - Integer.parseInt throws NumberFormatException on invalid input - String.substring validates range, ISO-8859-1 encoding maps unmappable chars to '?' - Vector.firstElement throws NoSuchElementException on empty vector
Runtime::get_file hands out cloned handles, so reads through fresh handles never advanced the file position. Share pos like the production File impls and make read honor it instead of consuming the buffer.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cd2eaf407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 79.46% 80.53% +1.07%
==========================================
Files 178 183 +5
Lines 7741 8018 +277
==========================================
+ Hits 6151 6457 +306
+ Misses 1590 1561 -29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple JVM/classfile parsing and Java runtime correctness bugs uncovered via codebase-wide analysis, and adds regression tests to prevent reintroductions.
Changes:
- Fix classfile opcode parsing alignment for
invokeinterface/invokedynamicand add targeted parser tests. - Fix JVM runtime type checks for
instanceof/checkcastwith interfaces and correctputstaticnarrowing for small integral field types. - Fix several
java_runtimebehavioral bugs (exceptions,substring, charset encoding,skip,mark/reset,Vector.firstElement) and improve test utilities’ file-handle semantics.
Reviewed changes
Copilot reviewed 34 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_putstatic.rs | Adds regression test ensuring putstatic narrows stack ints to the declared field type. |
| tests/test_helper/mod.rs | Removes a crate-level dead_code allow in test helper module. |
| test_utils/src/lib.rs | Fixes DummyFile cloning semantics by sharing position across clones and honoring seek. |
| test_data/unit/StaticFlag.java | Adds unit-test fixture class for static small-type fields. |
| test_data/InterfaceCast.txt | Adds expected output for new interface cast E2E test. |
| test_data/InterfaceCast.java | Adds E2E program covering instanceof/interface cast behavior. |
| jvm/tests/test_string.rs | Adds regression test for unpaired surrogate handling in string conversion. |
| jvm/tests/test_is_instance.rs | Adds regression test for is_instance across interface + superclass chains. |
| jvm/src/runtime/java_lang_string.rs | Makes Java→Rust UTF-16 conversion lossy to avoid panics on invalid UTF-16. |
| jvm/src/jvm.rs | Extends inheritance checks to include direct interfaces and loaded superinterfaces. |
| jvm/src/class_definition.rs | Extends ClassDefinition API with interface_names() (default empty). |
| jvm_rust/src/interpreter.rs | Shares putfield/putstatic narrowing logic via to_field_type helper. |
| jvm_rust/src/class_definition.rs | Preserves interface names when building class definitions from protos/classfiles. |
| java_runtime/tests/classes/java/util/test_vector.rs | Adds tests for Vector.firstElement() empty behavior and indexOf(null). |
| java_runtime/tests/classes/java/lang/test_string.rs | Adjusts floating test constant and adds tests for invalid substring + ISO-8859-1 unmappable chars. |
| java_runtime/tests/classes/java/lang/test_integer.rs | Adds test asserting parseInt throws NumberFormatException on invalid input. |
| java_runtime/tests/classes/java/io/test_file_input_stream.rs | Adds tests ensuring sequential reads advance position and skip returns actual skipped count. |
| java_runtime/tests/classes/java/io/test_data_input_stream.rs | Adds test covering InputStream.skip() behavior via DataInputStream. |
| java_runtime/tests/classes/java/io/test_byte_array_input_stream.rs | Adds test for ByteArrayInputStream.mark/reset correctness. |
| java_runtime/tests/classes/java/io/mod.rs | Registers new ByteArrayInputStream test module. |
| java_runtime/src/loader.rs | Registers newly added runtime exception classes in the runtime loader. |
| java_runtime/src/classes/java/util/vector.rs | Changes Vector.firstElement() empty behavior to throw NoSuchElementException. |
| java_runtime/src/classes/java/util/no_such_element_exception.rs | Implements java/util/NoSuchElementException runtime class. |
| java_runtime/src/classes/java/util.rs | Wires NoSuchElementException into the java.util module exports. |
| java_runtime/src/classes/java/lang/string.rs | Adds substring range validation + exception, and fixes ISO-8859-1 encoding for >0xFF chars. |
| java_runtime/src/classes/java/lang/string_index_out_of_bounds_exception.rs | Implements java/lang/StringIndexOutOfBoundsException runtime class. |
| java_runtime/src/classes/java/lang/number_format_exception.rs | Implements java/lang/NumberFormatException runtime class. |
| java_runtime/src/classes/java/lang/integer.rs | Makes Integer.parseInt throw NumberFormatException instead of panicking. |
| java_runtime/src/classes/java/lang.rs | Wires new java.lang exception classes into the module exports. |
| java_runtime/src/classes/java/io/input_stream.rs | Fixes InputStream.skip() to bound allocations and return actual skipped bytes. |
| java_runtime/src/classes/java/io/byte_array_input_stream.rs | Fixes ByteArrayInputStream.mark() to store current position (not readlimit). |
| classfile/tests/test.rs | Adds opcode-offset regression test for invokeinterface parsing alignment. |
| classfile/src/opcode.rs | Fixes parsing to consume all operand bytes for invokedynamic/invokeinterface; adds unit tests. |
| Cargo.toml | Adds workspace test_utils as a dev-dependency for integration tests. |
| Cargo.lock | Updates lockfile for added dev-dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- substring uses UTF-16 code unit indices like Java String.length() - US-ASCII encoding replaces non-ASCII chars instead of acting as latin-1 - clamp DummyFile position in u64 before casting to usize
Resolving interfaces alongside the superclass guarantees the whole interface hierarchy is registered, so is_inherited_from can look up transitive superinterfaces unconditionally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 10 bugs found during a codebase-wide analysis, each with a failing test written first (TDD), plus a test-infrastructure fix.
classfile
Interface.classE2E test only passed by accident; the new parser test asserts exact opcode offsets.jvm / jvm_rust
instanceof/checkcastalways failed against interfaces: interface names were dropped during class loading andis_inherited_fromonly walked the superclass chain.ClassDefinitionnow exposesinterface_names()and the check covers direct interfaces, loaded superinterfaces, and the superclass chain. Covered by a unit test and a newInterfaceCastE2E class.putstaticstored raw stack ints into boolean/byte/char/short fields, panicking on later typed reads from Rust. It now narrows by field descriptor likeputfield(sharedto_field_typehelper).JavaLangString::to_rust_stringpanicked on unpaired surrogates (legal in Java strings); now usesfrom_utf16_lossy.java_runtime
ByteArrayInputStream.mark()storedreadlimitinstead of the current position, corruptingreset().InputStream.skip()always returnednand allocated ann-sized scratch array; now loops with a bounded buffer and returns the actual skipped count.Integer.parseInt()panicked on invalid input; now throwsNumberFormatException.String.substring(begin, end)underflowed on invalid ranges; now throwsStringIndexOutOfBoundsException.?like the JDK.Vector.firstElement()returned null on an empty vector; now throwsNoSuchElementException.New runtime classes:
NumberFormatException,StringIndexOutOfBoundsException,NoSuchElementException.test_utils
DummyFiledeep-copied its state on clone, butRuntime::get_filehands out cloned handles — so file position never persisted across reads. It now shares position viaArc<Mutex>like the productionFileimpls, andreadhonorsseek.Test plan
cargo test --workspace: all 25 targets pass.cargo clippy --workspace --all-targets: no new warnings (one pre-existingapprox_constanterror attest_string.rs:309from Implement WIPI String API #155 remains).